-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New implementation of CMS_WCHARM_13TEV_WPWM-TOT-UNNORM #2244
Conversation
Hi @RoyStegeman , I think I'm done here. Please, see the following comments:
Honestly, I can't judge whether these differences are relevant or not. The difference in chi2 is not negligible if one accounts for the shifts. On the other hand, the difference in the t0 matrices does not really worry me as I was able to reproduce the chi2 of the legacy implementation provided shifts were removed. @RoyStegeman, what do you think? Maybe it is worth asking @enocera. |
Do you know why the fktables of this dataset only exist in theories 704 (0.5,0.5) and 705 (0.5,1)? |
No, maybe @enocera does. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so familiar with dataset implementations so this is going to take me some time to figure out...
For now I just have a question regarding the Extractor
class. There are also a lot of unused imports, are you using an lsp?
nnpdf_data/nnpdf_data/commondata/CMS_WCHARM_13TEV/filter_utils.py
Outdated
Show resolved
Hide resolved
nnpdf_data/nnpdf_data/commondata/CMS_WCHARM_13TEV/filter_utils.py
Outdated
Show resolved
Hide resolved
nnpdf_data/nnpdf_data/commondata/CMS_WCHARM_13TEV/sys_uncertainties.py
Outdated
Show resolved
Hide resolved
|
language server protocol. It's the software that highlights tokens based on their role in the python syntax. Including unused imports I'm pretty sure you are using it, but just in case you're not |
Oh, then I am. But I just forgot to delete the unused imports. |
I think that the reason is as follows: W+c data were not included in NNDPF4.0 because, at that time, NNLO corrections to the matrix elements were not known. When the MHOU, QED, and aN3LO determinations were produced, the leitmotiv was to put them on the same grounds as NNPDF4.0. Therefore W+c did not go into them. At some point I raised the question whether we should include it (in the same way as we do, e.g. for LHC data in the N3LO fit). Initially their answer was yes, but then they retracted. So I suspect that Andrea started to compute the FK tables, but then stopped. |
f054206
to
342e1f5
Compare
According to what ERN said in the last code meeting, this one is also ready for review. |
5ce9c42
to
0eb48b5
Compare
35eced4
to
3dd2bc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not have any active role in the fit. For that reason, every bin has the | ||
same value. Moreover, only the mid value is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean to say here. The fact that MW2
is the same for all bins is because it's a fixed parameter, whether it's used in the fit or not. Perhaps the point here is that even though it's fixed you still need to put the value in each bin because it's treated as a kinematic variable and the code needs to take those from the kinematics.yaml file (with the exception of sqrts, as you mentioned in the other comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean here is that MW2 is used only in the computation
nnpdf_data/nnpdf_data/commondata/CMS_WCHARM_13TEV/filter_utils.py
Outdated
Show resolved
Hide resolved
variants: | ||
legacy: | ||
data_uncertainties: | ||
- uncertainties_legacy_WPWM-TOT-UNNORM.yaml | ||
legacy_10: | ||
data_uncertainties: | ||
- uncertainties_WPWM-TOT-UNNORM_sys_10.yaml | ||
data_central: data_legacy_WPWM-TOT-UNNORM.yaml | ||
- uncertainties_legacy_WPWM-TOT-UNNORM_sys_10.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, one question: should the legacy
variant not also include the old central values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Yes, it should, given that it has changed. I'll restore it and add it to the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included the legacy central data in the variants. I've also updated the report in the description for the default legacy implementation.
f0d272d
to
e83aac3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, once the tests pass this can be merged
e83aac3
to
be779ee
Compare
This PR implements CMS_WCHARM_13TEV_WPWM-TOT-UNNORM in the new format.
General comments
This dataset delivers the differential distribution in function of the absolute rapidity of the lepton pair. Each data point is accompanied by a (symmetric) statistical uncertainty and a (asymmetric) systematical uncertainty. The latter is the sum in quadrature of the different sources of uncertainty. The breakdown of these systematic sources is not delivered in the HepData format, but it is given in Table 1 of the paper.
The legacy version has the variant
sys_10
, which should not be implemented because it was meant to account for the 3pt prescription.Legacy: [default],
New: [default w/ shifts], [default w/o shifts]